Support Range requests for multi-file ROM downloads#3167
Open
tmgast wants to merge 8 commits intorommapp:masterfrom
Open
Support Range requests for multi-file ROM downloads#3167tmgast wants to merge 8 commits intorommapp:masterfrom
tmgast wants to merge 8 commits intorommapp:masterfrom
Conversation
Cache ZIPs to disk on Range requests so nginx can serve 206 natively. Non-Range requests still stream via mod_zip as before. Refs: rommapp#3160
- Use ZipFile.write() instead of reading entire files into memory - Extract magic numbers to module constants - Use hl() for log message highlighting - Tests for zip_cache, m3u, and cleanup task using mocker fixture
- Bulk /download endpoint now caches ZIPs for Range requests (max 100 ROMs)
- LRU eviction frees space by removing oldest cached ZIPs (>1h old)
- Tiered TTL: 48h default, 12h for ZIPs over 8GB
- Generalize cache paths from rom_id to namespace (supports bulk-{ids})
A third-party library in the import chain replaces zipfile._get_compressor with an incompatible signature. Reload the zipfile module before building cached ZIPs to restore it.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds HTTP Range/resume support for multi-file ROM downloads (and bulk /download) by building deterministic, on-disk cached ZIPs and serving them via nginx X-Accel-Redirect, while keeping the existing mod_zip streaming path for non-Range requests.
Changes:
- Add nginx internal route for cached ZIP delivery (
/cache/→${ROMM_BASE_PATH}/cache/). - Introduce ZIP cache utilities (keying, build-to-disk, eviction, TTL cleanup) and extract shared M3U playlist generation.
- Add a scheduled cleanup task and register it for startup + tasks endpoint visibility.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/nginx/templates/default.conf.template | Adds internal /cache/ alias used by X-Accel-Redirect to serve cached ZIPs with native Range support. |
| backend/utils/zip_cache.py | Implements cache keying, disk-space checks/eviction, ZIP building, redirect path, and TTL-based cleanup. |
| backend/utils/m3u.py | Extracts shared M3U playlist generation logic for multi-file ROMs. |
| backend/tasks/scheduled/cleanup_zip_cache.py | Adds daily scheduled task to delete stale cached ZIPs. |
| backend/startup.py | Initializes the new scheduled ZIP cache cleanup task on startup. |
| backend/endpoints/tasks.py | Exposes the ZIP cache cleanup task in the scheduled tasks list. |
| backend/endpoints/roms/init.py | Adds Range-aware cache/redirect behavior for multi-file ROM downloads and bulk downloads; uses shared M3U generator. |
| backend/config/init.py | Adds ZIP_CACHE_PATH configuration constant. |
| backend/tests/utils/test_zip_cache.py | Adds unit tests for zip cache behavior (keying, build, TTL cleanup, eviction). |
| backend/tests/utils/test_m3u.py | Adds unit tests for M3U content generation behavior. |
| backend/tests/tasks/test_cleanup_zip_cache.py | Adds unit tests for the new scheduled cleanup task. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use zipfile.ZipFile after reload instead of stale direct import - Use ZipFile.write() for streaming (reload fixes CPython 3.13 bug) - Wrap cache build in try/except, fall back to mod_zip on failure - Guard empty entries list in get_cache_key - Hash bulk namespace to avoid ENAMETOOLONG with 100 ROM IDs - Use MagicMock for sync unschedule method in tests - Mock _get_available_space in eviction tests to force eviction path - Use sparse file (truncate) for large file TTL test - Add tests for get_bulk_namespace and empty entries
gantoine
removed
the
on-hold
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds HTTP Range request support for multi-file ROM downloads by caching assembled ZIPs to disk on demand. When a client sends a
Rangeheader, the server builds a static ZIP and serves it viaX-Accel-Redirect, letting nginx handle206 Partial Contentnatively. Non-Range requests continue to stream viamod_zipas before.This also applies to the bulk
/downloadendpoint for up to 100 ROMs.How it works
Range: bytes=0-on a multi-file ROM downloadROMM_BASE_PATH/cache/zips/{namespace}/{key}.zipFileRedirectResponsewithX-Accel-Redirectpointing to the cached fileContent change safety
nginx auto-generates ETags on static files. When content changes (re-scan), a new cache key produces a new file with a new ETag. Clients resuming with
If-Range+ the old ETag get200(full content) per RFC 7233, signaling they should restart. Old cache files are preserved for in-progress downloads and cleaned up by the scheduled task.Tiered TTL and eviction
When disk space is insufficient for a new cache entry, the oldest cached ZIPs (over 1 hour old) are evicted LRU-style until enough space is available or no evictable entries remain. If still insufficient, the request falls back to
mod_zipstreaming.A 2x disk space buffer is required before building a new cached ZIP.
Endpoints affected
GET /{id}/content/{name}HEAD /{id}/content/{name}Accept-Ranges: bytes+Content-Lengthwhen cache existsGET /downloadKnown gaps
DEV_MODE=true) builds in-memory ZIPs and does not reach the Range/cache code path. Range support is production-only (requires nginx).RomFile.updated_attracks DB row modification, not file content change. Files replaced on disk without re-scanning may serve a stale cached ZIP until the next scan.zipfile stdlib override
A third-party library in the import chain replaces
zipfile._get_compressorwith a version that has an incompatible call signature, breakingZipFile.writestr()andZipFile.write()on CPython 3.13. The cache builder works around this by reloading thezipfilemodule before use. This is a pre-existing issue that also affects the dev-mode in-memory ZIP builder.New files
backend/utils/zip_cache.pybackend/utils/m3u.pybackend/tasks/scheduled/cleanup_zip_cache.pyChecklist